Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(utils): use test instead of indexof in stacktrace #7417

Merged
merged 4 commits into from
Mar 14, 2023

Conversation

JonasBa
Copy link
Member

@JonasBa JonasBa commented Mar 10, 2023

We dont require the actual index + we can match with a single fn call

@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.23 KB (+0.05% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 62.93 KB (+0.06% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.86 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55.84 KB (+0.07% 🔺)
@sentry/browser - Webpack (gzipped + minified) 20.61 KB (+0.07% 🔺)
@sentry/browser - Webpack (minified) 67.36 KB (+0.05% 🔺)
@sentry/react - Webpack (gzipped + minified) 20.64 KB (+0.08% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.53 KB (+0.16% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.49 KB (+0.16% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.75 KB (+0.15% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.17 KB (0%)
@sentry/replay - Webpack (gzipped + minified) 37.19 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 61.16 KB (+0.06% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 54.34 KB (+0.01% 🔺)

.reverse();
return localStack.map(frame => ({
...frame,
filename: frame.filename || localStack[0].filename,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit puzzled why this has a fallback to localStack[0].filename?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure - this seems strange to me, and feels wrong. Shouldn't we just drop the filename if it's not set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is what I would assume. It seems like we were defaulting to the file at the root of the stack?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess for now let's just keep the behaviour, but I'll make a TODO to come back and investigate this.

@JonasBa
Copy link
Member Author

JonasBa commented Mar 10, 2023

OK, I'm having some trouble with fixing this test... Even if I revert my implementation back to the original it still fails locally? I'm not entirely sure why this would be the case or if I'm maybe using this wrongly? I noticed that since the test depends on a different package I wanted to rebuild the package however I still cant see any logs 😢

@AbhiPrasad
Copy link
Member

Let's merge this in for now and come back to take a look at the filename.

@AbhiPrasad AbhiPrasad merged commit a34dab6 into develop Mar 14, 2023
@AbhiPrasad AbhiPrasad deleted the jb/ref/stacktrace-matching branch March 14, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants